Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HA State: ensure not this and another instance can be responsible #825

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Oct 11, 2024

In theory, this should not happen. This assumption is based on the trust in the database transaction performing the HA realization logic. However, one debugged log let one assume that this happened anyway.

This change mostly signals an error while also explicitly giving up the HA state. Doing so should at least alarm a person reading the logs.


ref/IP/55850

@oxzi oxzi added bug Something isn't working ref/IP labels Oct 11, 2024
@oxzi oxzi requested a review from lippserd October 11, 2024 15:08
@cla-bot cla-bot bot added the cla/signed label Oct 11, 2024
@oxzi oxzi requested a review from julianbrost October 11, 2024 15:24
In theory, this should not happen. This assumption is based on the trust
in the database transaction performing the HA realization logic.
However, one debugged log let one assume that this happened anyway.

This change mostly signals an error while also explicitly giving up the
HA state. Doing so should at least alarm a person reading the logs.
@oxzi oxzi force-pushed the ha-state-both-responsible-reject branch from 90c366b to a38491a Compare October 28, 2024 11:56
@oxzi
Copy link
Member Author

oxzi commented Oct 28, 2024

For the record, this behavior was seen in the linked ticket a short time after something in the database cluster went south.

Furthermore, I have rebased the branch to reflect the latest changes on the main branch.

@oxzi oxzi added this to the 1.3.0 milestone Oct 31, 2024
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oxzi If I'm not mistaken, the changes introduced were not triggered (anymore) after #800 was used in the referenced ticket, right? Anyway, a little more protection can't hurt.

@oxzi
Copy link
Member Author

oxzi commented Dec 16, 2024

@oxzi If I'm not mistaken, the changes introduced were not triggered (anymore) after #800 was used in the referenced ticket, right? Anyway, a little more protection can't hurt.

The build in the mentioned ticket contains both this branch as well as #800.

So far, I cannot think of a valid situation which will break with this change, as it addresses a previously under-specified state.

@lippserd
Copy link
Member

lippserd commented Dec 16, 2024

The build in the mentioned ticket contains both this branch as well as #800.

Yes, I know. I meant that with #800 there were no (more) logs that say "Other instance is responsible...", right? So I wanted to question whether this change is actually necessary, but at the same time say that more protection can't hurt.

@lippserd lippserd removed the request for review from julianbrost December 16, 2024 14:24
@lippserd lippserd merged commit 5454c89 into main Dec 16, 2024
32 checks passed
@lippserd lippserd deleted the ha-state-both-responsible-reject branch December 16, 2024 16:01
@oxzi
Copy link
Member Author

oxzi commented Dec 16, 2024

The build in the mentioned ticket contains both this branch as well as #800.

Yes, I know. I meant that with #800 there were no (more) logs that say "Other instance is responsible...", right? So I wanted to question whether this change is actually necessary, but at the same time say that more protection can't hurt.

Unfortunately, there was no deployment of only #800, but always of these two PRs combined. Furthermore, I never got detailed enough logs to be able to answer this question. Thus, at least in this very scenario, both branches combined seemed to solve or at least mitigate the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed ref/IP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants